Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor setup and modernize package structure as well as CI #287

Merged
merged 34 commits into from
Jul 15, 2022

Conversation

MuellerSeb
Copy link
Contributor

@MuellerSeb MuellerSeb commented Jun 30, 2022

CHANGES

TODOs

  • add test.pypi.org account to check pypi upload for all pushes to master
  • coverage is not yet sent to coveralls to not produce an error
  • decide: should black and isort be applied?
  • decide: should hymod executables be part of the sdist and wheel?
  • decide: should the cli be installed as a script?
  • before merging: remove "refactor_setup" from CI (was added for testing here)
  • future tags should follow semver: v1.6.0 or v1.6.0-rc1

NOTES

Since I introduced setuptools_scm to derive the version, there is no need to put the version by hand anywhere.

Pros

  • version file _version.py will be generated automatically when installing with pip (even editable), or when building sdist / wheel
  • development version will be derived from commit distance to last tag (i.e. 1.5.17.dev18)
  • to get a new version, simply add a new tag following semver
    • prefixed v in tag
    • always use major.minor.patch: i.e. v1.5.18
    • you can also release beta and release candidate versions: v1.5.18-beta, v1.5.18-rc1

Cons

  • installing from zip archive results in fallback version 0.0.0.dev0 (see here for a solution, but IMHO it's a corner case that should be discouraged)
  • but installing from git repo still works:
    pip install git+https://github.com/thouska/spotpy.git

@MuellerSeb MuellerSeb changed the title Refactor setup Refactor setup and modernize package structure as well as CI Jul 1, 2022
@thouska
Copy link
Owner

thouska commented Jul 6, 2022

Hey @MuellerSeb,
wow what an amazing pull request. There was obviously a lot of clean-up necessary. In general, I think this pull request is excellent and will bring a lot of benefits. I like the new structure, testing features, and automatized version control.
The only thing I would like to change back is to transfer the tutorial with the installation of the package. May not be the most elegant way, but as there are not so many files, I think it is reasonable.
Regarding the ToDos, I can comment on the following:

add test.pypi.org account to check PyPI upload for all pushes to master
I added the missing Tokens in .github/workflows/main.yml and hope this is what you meant?

coverage is not yet sent to coveralls to not produce an error
I also added this Token, so I think we could also push to coveralls, can't we?

decide: should black and isort be applied?
Yes, I think that would be good

decide: should hymod executables be part of the sdist and wheel?
Yes, would be good, also applies to the tutorial files

decide: should the cli be installed as a script?
I guess so. Are there other options?

before merging: remove "refactor_setup" from CI (was added for testing here)
Ok, will do so, once we merge

pyproject.toml Outdated Show resolved Hide resolved
@MuellerSeb
Copy link
Contributor Author

MuellerSeb commented Jul 8, 2022

Hey @MuellerSeb, wow what an amazing pull request. There was obviously a lot of clean-up necessary. In general, I think this pull request is excellent and will bring a lot of benefits. I like the new structure, testing features, and automatized version control. The only thing I would like to change back is to transfer the tutorial with the installation of the package. May not be the most elegant way, but as there are not so many files, I think it is reasonable.

I would still argue against putting them into the spotpy namespace. If they are under the src/spotpy folder, these tutorial scripts could be imported, but that would just execute them which doesn't make sense in contrast to the example setups, which should be there (as I did it). My solution would be to at least include the tutorials in the source distribution by adding the folder to the MANIFEST.in, since there it makes sense. When installing spotpy having the tutorials in the namespace it is also not that easy getting to the scripts, since they are buried in the installation and system dependent python package location.

Regarding the ToDos, I can comment on the following:

add test.pypi.org account to check PyPI upload for all pushes to master
I added the missing Tokens in .github/workflows/main.yml and hope this is what you meant?

Cool!

coverage is not yet sent to coveralls to not produce an error
I also added this Token, so I think we could also push to coveralls, can't we?

Also cool!

decide: should black and isort be applied? Yes, I think that would be good

Will do so!

decide: should hymod executables be part of the sdist and wheel?
Yes, would be good, also applies to the tutorial files

Check!

decide: should the cli be installed as a script?
I guess so. Are there other options?

I would shift that to a later pull request, since that would make click a hard dependency.

before merging: remove "refactor_setup" from CI (was added for testing here)
Ok, will do so, once we merge

I can also reset that short before merging.

@thouska
Copy link
Owner

thouska commented Jul 11, 2022

I would still argue against putting them into the spotpy namespace.

You are right, separating the spot_setup example from the tutorials makes sense. With your restructuring, it is easy to just plug and play the tutorials and load the examples from the source code. It also makes sense, as at some point in the future I planed to make jupyter notebooks out of these tutorials, which also do not belong into the source code. Thank you for this suggestion!

decide: should the cli be installed as a script?
I guess so. Are there other options?
I would shift that to a later pull request, since that would make click a hard dependency.

Ok, let's wait with this one and push this pull request first

@thouska
Copy link
Owner

thouska commented Jul 13, 2022

So good to see all the new tests and features! Is there anything missing for the coveralls testing at this point?

@MuellerSeb
Copy link
Contributor Author

@thouska I think we are almost ready. If I add the coveralls upload and prepare the CI script for the merge, the CI on my side will not run. But we should that then test later on in this repository.

I think this PR is ready. Other stuff should be done later on in separate PRs:

  • should hymod be compiled with cython? (got some experience with that)
  • update CHANGELOG.md
  • update CONTRIBUTING.md
  • add GUI script as command during installation

One last question: Can I add myself to the author list? 😉

@MuellerSeb
Copy link
Contributor Author

@thouska added the missing github token env var.

@thouska
Copy link
Owner

thouska commented Jul 13, 2022

should hymod be compiled with cython? (got some experience with that)

I think hymod.py is currently compiled with jit, so I wouldn't expect too much more speed potential with cython, but feel free to surprise me :)

update CHANGELOG.md

I will do that, once I tag the merged files. Super curious to see, whether this will automatically result in a new pypi version. That would be pure magic :D

update CONTRIBUTING.md

I guess you have something in mind there?

add GUI script as command during installation

Spontaneously, I wouldn't know how to do that

One last question: Can I add myself to the author list? 😉

Yes, you can :)

@MuellerSeb
Copy link
Contributor Author

MuellerSeb commented Jul 13, 2022

should hymod be compiled with cython? (got some experience with that)

I think hymod.py is currently compiled with jit, so I wouldn't expect too much more speed potential with cython, but feel free to surprise me :)

Ok. I am just wondering about the unix version (I guess the exe version is working on most windows versions). There is a makefile to compile the cython extension. Would it maybe make sense to put hymod into a separate python package? I don't think it is a good idea to have a makefile in the source of spotpy.

update CHANGELOG.md

I will do that, once I tag the merged files. Super curious to see, whether this will automatically result in a new pypi version. That would be pure magic :D

I always update the changelog as the last thing before the tag, so the correct changelog is part of the release.
Also, a develop version should be sent to test.pypi for each push to master.

It could a good idea, to create a release candidate as a pre-release with v1.6.0-rc1 (this works well with pypi, since it recognizes these versions as pre-release and pip install spotpy will not use them (but could be set with a command line argument), see here for an example with gstools from this tag)

As just written, I would go to 1.6.0 after this merge, since py<3.7 was dropped.

update CONTRIBUTING.md

I guess you have something in mind there?

Update info about black and isort. Also it could be good to implement pylint in the future and add info about it there as well.

add GUI script as command during installation

Spontaneously, I wouldn't know how to do that

No problem. This is rather easy.

One last question: Can I add myself to the author list? wink

Yes, you can :)

Whoop whoop. 🎉

Finally

This can be merged now. Everything else could be cleaned up later.

@philippkraft
Copy link
Collaborator

@MuellerSeb Thank you from my side also. Sorry for not responding earlier. What would you and @thouska think of moving hymod (and especially c-hymod) to another repo and make it an optional dependency of spotpy? That way, building spotpy from source would not require a compiler and deployment of new versions is simpler (no need for a binary release for each Python version, and the manylinux magic is not necessary).

@MuellerSeb
Copy link
Contributor Author

@MuellerSeb Thank you from my side also. Sorry for not responding earlier. What would you and @thouska think of moving hymod (and especially c-hymod) to another repo and make it an optional dependency of spotpy? That way, building spotpy from source would not require a compiler and deployment of new versions is simpler (no need for a binary release for each Python version, and the manylinux magic is not necessary).

That was my thought as well. in GSTools for example, we use wheels to distribute compiled extensions, so there is also no need to have a compiler at least for the most common systems and python versions (MacOS (x64 and arm), Windows (32bit+64bit), Linux (32bit+64bit) for py3.7 - py3.10. Since it is only a cython file, this is very easy to do and the compilation could be done in the CI using cibuildwheel.

What is the difference between hymod_unix, hymod_exe and hymod_python? (Maybe we could discuss this in an issue)

@thouska
Copy link
Owner

thouska commented Jul 15, 2022

All right, let's bring this good stuff to the master branch. Thousand thanks again for your contribution @MuellerSeb and your feedback @abravalheri! All remaining tasks that popped up during the pull request can be followed up on in #288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants